PM-31923 adding the whole report endpoints v2#7228
PM-31923 adding the whole report endpoints v2#7228prograhamming wants to merge 135 commits intomainfrom
Conversation
…-access-intelligence
…-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
…-access-intelligence
…-access-intelligence
* refactor(billing): update seat logic * test(billing): update tests for seat logic
…-access-intelligence
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Return WebAuthn credential record in create response * Make CreateWebAuthnLoginCredentialCommand null-safe
…#7123) * Remove emergency access from all organization users on policy enable, or when accepted/restored * Use correct policy save system * Add additional tests * Implement both PreUpsert and OnSave side effects
* Add coupon support to invoice preview and subscription creation * Fix the build lint error * Resolve the initial review comments * fix the failing test * fix the build lint error * Fix the failing test * Resolve the unaddressed issues * Fixed the deconstruction error * Fix the lint issue * Fix the lint error * Fix the lint error * Fix the build lint error * lint error resolved * remove the setting file * rename the variable name validatedCoupon * Remove the owner property * Update OrganizationBillingService tests to align with recent refactoring - Remove GetMetadata tests as method no longer exists - Remove Owner property references from OrganizationSale (removed in d761336) - Update coupon validation to use SubscriptionDiscountRepository instead of SubscriptionDiscountService - Add missing imports for SubscriptionDiscount entities - Rename test for clarity: Finalize_WithNullOwner_SkipsValidation → Finalize_WithCouponOutsideDateRange_IgnoresCouponAndProceeds All tests passing (14/14) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix the lint error * Making the owner non nullable * fix the failing unit test * Make the owner nullable * Fix the bug for coupon in Stripe with no audience restrictions(PM-32756) * Return validation message for invalid coupon * Update the valid token message * Fix the failing unit test * Remove the duplicate method * Fix the failing build and test * Resolve the failing test * Add delete of invalid coupon * Add the expired error message * Delete on invalid coupon in stripe * Fix the lint errors * return null if we get exception from stripe * remove the auto-delete change * fix the failing test * Fix the lint build error --------- Co-authored-by: Claude <noreply@anthropic.com>
feat: add MasterPasswordSalt column to User table - Add MasterPasswordSalt column to User table in both Dapper and EF implementations - Update User stored procedures (Create, Update, UpdateMasterPassword) to handle salt column - Add EF migrations and update UserView with dependent views - Set MaxLength constraint on MasterPasswordSalt column - Update UserRepository implementations to manage salt field - Add comprehensive test coverage for salt handling and normalization
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var fileData = report.GetReportFile(); | ||
| if (fileData == null) | ||
| { | ||
| throw new BadRequestException("Report ID in the request body must match the route parameter"); | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
🟡 IMPORTANT: Download endpoint serves unvalidated files (self-hosted)
DownloadReportFileAsync does not check fileData.Validated before serving the file. The cloud path in GetOrganizationReportAsync (line 212) and GetLatestOrganizationReportAsync (line 165) only exposes download URLs when Validated == true. Self-hosted instances would serve files that haven't passed size validation.
Consider adding:
| var fileData = report.GetReportFile(); | |
| if (fileData == null) | |
| { | |
| throw new BadRequestException("Report ID in the request body must match the route parameter"); | |
| throw new NotFoundException(); | |
| } | |
| var fileData = report.GetReportFile(); | |
| if (fileData == null || !fileData.Validated) | |
| { | |
| throw new NotFoundException(); | |
| } |
There was a problem hiding this comment.
Will be addressed in iterative work for file validation
| } | ||
|
|
||
| #endregion | ||
| } |
There was a problem hiding this comment.
🟡 IMPORTANT: Missing test coverage for UploadReportFileAsync and DownloadReportFileAsync
The two new self-hosted endpoints (UploadReportFileAsync and DownloadReportFileAsync) have zero test coverage. These endpoints contain authorization checks, org-ownership validation, file-size validation with cleanup-on-failure logic, and the validated-file guard that is currently missing (see other comment). Tests for these paths would help catch the inconsistencies flagged elsewhere in this review.
Additionally, the V2 create path's file-size cap (Constants.FileSize501mb rejection at controller line 114-116) is untested.
…-access-intelligence
Banrion
left a comment
Banrion
left a comment
There was a problem hiding this comment.
Only one blocking change request, the IDOR comment.
| if (fileData == null || fileData.Id != reportFileId) | ||
| { | ||
| throw new NotFoundException("Organization report data not found."); | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
@prograhamming I think this is worth following up on.
|
|
||
| #endregion | ||
|
|
||
| [HttpPatch("{organizationId}/data/application/{reportId}")] |
There was a problem hiding this comment.
📝 The reportId position in all routes is non-standard. It should be "{organizationId}/{reportId}/data/application". These are existing routes for v1 and currently matches client routes. No changes needed but wanted to call it out for future work
| if (request.ContentEncryptionKey != null) | ||
| { | ||
| existingReport.ContentEncryptionKey = request.ContentEncryptionKey; | ||
| } | ||
|
|
||
| if (request.SummaryData != null) | ||
| { | ||
| existingReport.SummaryData = request.SummaryData; | ||
| } | ||
|
|
||
| if (request.ApplicationData != null) | ||
| { | ||
| existingReport.ApplicationData = request.ApplicationData; | ||
| } |
…-access-intelligence
| } | ||
|
|
||
| public Task<string> GetReportFileUploadUrlAsync(OrganizationReport report, ReportFile fileData) | ||
| => Task.FromResult($"/reports/organizations/{report.OrganizationId}/{report.Id}/file/report-data"); |
There was a problem hiding this comment.
❌ CRITICAL: Upload URL path does not match the controller route — self-hosted uploads will 404
Details and fix
The controller upload endpoint was renamed in this PR from [HttpPost("{organizationId}/{reportId}/file/report-data")] to [HttpPost("{organizationId}/{reportId}/file")] (controller line 375), but this method still returns the old path with the /report-data suffix. There is no route registered for POST /reports/organizations/{orgId}/{reportId}/file/report-data, so every self-hosted client that follows this URL receives 404.
Additionally, UploadReportFileAsync reads reportFileId from the query string (line 379) and rejects the request if it is missing, but the generated URL does not include that query parameter.
| => Task.FromResult($"/reports/organizations/{report.OrganizationId}/{report.Id}/file/report-data"); | |
| public Task<string> GetReportFileUploadUrlAsync(OrganizationReport report, ReportFile fileData) | |
| => Task.FromResult($"{_apiBaseUrl}/reports/organizations/{report.OrganizationId}/{report.Id}/file?reportFileId={Uri.EscapeDataString(fileData.Id!)}"); |
Note the existing test GetReportFileUploadUrlAsync_ReturnsApiEndpoint at test/Core.Test/Dirt/Reports/Services/LocalOrganizationReportStorageServiceTests.cs:71 asserts the stale URL and will also need updating. This parallels the previously-fixed download URL mismatch (thread PRRT_kwDOAsltcc52eQH6) — the same rename needs to be applied here.
There was a problem hiding this comment.
To follow up on but not blocking for v1 regression testing. cc @prograhamming
…-access-intelligence
| [JsonPropertyName("metrics")] | ||
| public OrganizationReportMetrics? ReportMetrics { get; set; } | ||
| public long? FileSize { get; set; } | ||
|
|
There was a problem hiding this comment.
⛏️ Optional fields are Inconsistent with prior changes to remove optional fields from UpdateOrganizationReportV2RequestModel. These optional fields may be required for use in the create route for backwards compatibility with v1. To be reviewed and followed up on with dead null checks.
| public string? ReportFileDownloadUrl { get; set; } | ||
| public FileUploadType? FileUploadType { get; set; } |
There was a problem hiding this comment.
📝 These seem to be unused in OrganizationReportResponseModel below. When using this model the ReportFileDownloadUrl and FileUploadType. Are included alongside a OrganizationReportResponseModel in the response. Not blocking, but has potential to be consolidated. To be followed up on.
| { | ||
| _logger.LogWarning( | ||
| "Deleted report {ReportId} because its file size {Size} was invalid.", | ||
| report.Id, length); | ||
| await _storageService.DeleteReportFilesAsync(report, reportFileId); | ||
| await _organizationReportRepo.DeleteAsync(report); | ||
| return false; |
There was a problem hiding this comment.
I think this is addressed in the above lines
| } | ||
|
|
||
| public Task<string> GetReportFileUploadUrlAsync(OrganizationReport report, ReportFile fileData) | ||
| => Task.FromResult($"/reports/organizations/{report.OrganizationId}/{report.Id}/file/report-data"); |
There was a problem hiding this comment.
To follow up on but not blocking for v1 regression testing. cc @prograhamming
|
20 participants
🎟️ Tracking
This is a PR for user story PM-31923
📔 Objective
Creating new V2 endpoints for read and update operations on the whole report in the database. This will also include the logic for saving a reportData file in Azure Blob storage and server if self-hosted.
Documentation: